Skip to content

libcore/net: IpAddr::as_octets() #136609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 21, 2025
Merged

libcore/net: IpAddr::as_octets() #136609

merged 1 commit into from
Feb 21, 2025

Conversation

mammothbane
Copy link
Contributor

@mammothbane mammothbane commented Feb 6, 2025 •
edited
Loading

ACP
Tracking issue

Adds const core::net::IpAddr{,v4,v6}::as_octets() methods to provide reference access to IP address contents.

The concrete usecase for me is allowing the IpAddr to provide an extended lifetime in contexts that want a &[u8]:

trait AddrSlice {
    fn addr_slice(&self) -> &[u8];
}

impl AddrSlice for IpAddrV4 {
    fn addr_slice(&self) -> &[u8] {
        // self.octets() doesn't help us here, because we can't return a reference to the owned array.
        // Instead we want the IpAddrV4 to continue owning the memory:
        self.as_octets()
    }
}

(Notably, in this case we can't parameterize AddrSlice by a const N: usize (such that fn addr_slice(&self) -> [u8; N]) and maintain object-safety.)

@rustbot
Copy link
Collaborator

rustbot commented Feb 6, 2025

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 6, 2025
@scottmcm scottmcm added needs-acp This change is blocked on the author creating an ACP. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2025
@scottmcm
Copy link
Member

scottmcm commented Feb 6, 2025

If you wish to add a new API, please open an ACP to get approval for it first: https://std-dev-guide.rust-lang.org/development/feature-lifecycle.html#suitability-for-the-standard-library

@scottmcm
Copy link
Member

scottmcm commented Feb 6, 2025

Looking at this myself, why -> &[u8] on the known-length ones? Should it be IpAddr::as_array : &self -> &[u8; 4] instead?

(But questions like that are what the ACP review is for.)

@mammothbane
Copy link
Contributor Author

Hmm, yeah, I guess I actually only want to erase length information for the sake of providing AsRef<[u8]> — these methods can include it.

I hoped this was a minimal enough change that an ACP wasn't required, but I'm happy to open one. (Maybe this weekend.)

@scottmcm
Copy link
Member

scottmcm commented Feb 6, 2025 •
edited
Loading

Generally I ask for an ACP in the vast majority of the cases, since for new stuff I don't know what libs-api might think. Maybe they'd just say to add AsRef directly, for example.

The common exceptions to having an ACP are things like

  • Adding something that's obviously related to an existing unstable feature, under that same gate, where you can say "well the suitability check was met by the existing ACP" if there's something found in nightly that changes how it should be done compared to what was in the ACP.
  • Adding something that's an "obvious miss" compared to something else -- like if IpAddrV6 had segments but somehow IpAddrV4 didn't have octets, that'd be such an obvious parallel that I'd probably just accept it.

But here there might also be some reason that the & versions have byte order concerns, or a discussion of whether there should be a segment form for v6, or ...

@mammothbane
Copy link
Contributor Author

Got it, yep — happy to open one. I appreciate the explanation / reasoning.

@mammothbane mammothbane mentioned this pull request Feb 8, 2025
Closed
@scottmcm
Copy link
Member

Looks like the methods were accepted rust-lang/libs-team#535 (comment) as as_octets, so flipping back to
@rustbot author

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 15, 2025
@scottmcm scottmcm removed the needs-acp This change is blocked on the author creating an ACP. label Feb 15, 2025
@mammothbane mammothbane changed the title libcore/net: IpAddr::as_slice() libcore/net: IpAddr::as_octets() Feb 18, 2025
@mammothbane
Copy link
Contributor Author

Updated with ACP's requested changes — if I understand the process correctly, this wants a tracking issue also?

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 18, 2025
scottmcm
scottmcm requested changes Feb 18, 2025
View reviewed changes
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 18, 2025
@mammothbane mammothbane mentioned this pull request Feb 19, 2025
Open
4 tasks
@mammothbane
core/net: IpAddr*::as_octets()
8fb8885 
Adds `const` `Ip*Addr::as_octets` methods providing reference access to
`Ip*Addr` octets contents.

See rust-lang/libs-team#535 for accepted ACP
with a more detailed justification.
@mammothbane
Copy link
Contributor Author

Done!

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 19, 2025
@scottmcm
Copy link
Member

Thanks!
@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Feb 21, 2025

📌 Commit 8fb8885 has been approved by scottmcm

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 21, 2025
Merged
@bors bors merged commit 4aa973b into rust-lang:master Feb 21, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottmcm scottmcm scottmcm requested changes

Assignees

@scottmcm scottmcm

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.87.0
Development

Successfully merging this pull request may close these issues.
4 participants
@mammothbane @rustbot @scottmcm @bors